Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: new cli install fragment #1139

Merged
merged 13 commits into from
Dec 20, 2022
Merged

Conversation

quetzalliwrites
Copy link
Member

Take 2 from #1113

@quetzalliwrites quetzalliwrites added 📑 docs area/docs Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. gsod This label should be used for issues or discussions related to ideas for Google Season of Docs labels Dec 2, 2022
@quetzalliwrites quetzalliwrites self-assigned this Dec 2, 2022
@netlify
Copy link

netlify bot commented Dec 2, 2022

Deploy Preview for asyncapi-website ready!

Name Link
🔨 Latest commit f5e08e6
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/63a184130a66250009ad07e5
😎 Deploy Preview https://deploy-preview-1139--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@quetzalliwrites
Copy link
Member Author

what if we just merge this one instead? 😄

cc @magicmatatjahu @derberg

@github-actions
Copy link

github-actions bot commented Dec 2, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 37
🟠 Accessibility 88
🟢 Best practices 100
🟢 SEO 100
🔴 PWA 30

Lighthouse ran on https://deploy-preview-1139--asyncapi-website.netlify.app/

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only have 2 questions:

  • how is this fragment going to be used? Is the assumption that the intro to CLI and reference to GitHub repo are done in a separate section? I'm asking as, after rendering, it looks weird without an intro
    Screenshot 2022-12-05 at 13 01 30
  • we are missing instructions on how to install on any operating system using NPM (Using NPM and Node)

@magicmatatjahu
Copy link
Member

@alequetzalli In general I accept the changes, but let it be accepted by Łukasz, because I see that he sees the problems and knows better what this fragment should have :)

@quetzalliwrites
Copy link
Member Author

  • how is this fragment going to be used?

All CLI installation guide H3 fragments will be placed at the end of the doc's Installation Guide H2 section.

EX: If we were adding this fragment to the current Streetlights tutorial, it would look like this...

import CliInstallation from '../../../assets/docs/fragments/cli-installation.md' 

<CliInstallation/>
  • we are missing instructions on how to install on any operating system using NPM (Using NPM and Node)

added! ✅

@quetzalliwrites
Copy link
Member Author

ready for review again @derberg @magicmatatjahu

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@magicmatatjahu In this PR @alequetzalli uses ### and ####

but in the preview they look almost identical. Shouldn't h4 have class text-md instead of text-lg?

<h3 id="cli-installation" class=" mb-4 mt-6 font-heading antialiased font-medium tracking-heading text-gray-900 text-lg">CLI Installation</h3>

Screenshot 2022-12-12 at 15 32 21

For ToC we have it right 😄 easy to see it is sub section

Screenshot 2022-12-12 at 15 32 35

assets/docs/fragments/cli-installation.md Outdated Show resolved Hide resolved
@magicmatatjahu
Copy link
Member

@derberg Sure, we can change it, but fyi h5 also uses text-md. https://github.com/asyncapi/website/blob/master/components/MDX.js#L41 So we also have to remember about that.

quetzalliwrites and others added 2 commits December 15, 2022 16:43
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@quetzalliwrites
Copy link
Member Author

so ...what are we doing about the header's perceived font size then? leaving as is or...?
@derberg @magicmatatjahu

@derberg
Copy link
Member

derberg commented Dec 19, 2022

@alequetzalli I think it is more important to visibly distinguish between h3 and h4 than h4 and h5. Because I think h5 is not that often used anyway

so please in this PR also make a change in https://github.com/asyncapi/website/blob/master/components/MDX.js#L41

  • from
    h4: props => <h4 {...props} className={`${props.className || ''} my-4 font-heading antialiased font-medium text-lg text-gray-900`} />,
    
  • to
    h4: props => <h4 {...props} className={`${props.className || ''} my-4 font-heading antialiased font-medium text-md text-gray-900`} />,
    

@quetzalliwrites
Copy link
Member Author

@alequetzalli I think it is more important to visibly distinguish between h3 and h4 than h4 and h5. Because I think h5 is not that often used anyway

@derberg omg YAY, thanks for showing me where to make that style change, I had no idea 😂

Yeah I agree, I'm glad you caught this and noticed the sizing differences, I did not.

@quetzalliwrites
Copy link
Member Author

okidoki, I have completed the latest feedback from @derberg ✨✨✨✨

READY FOR REVIEW AGAIN jumps up and down

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done 👏🏼

Ready to merge first fragment 🚀 Hail to reusability 🎆

@magicmatatjahu anything from your side?

Copy link
Member

derberg commented Dec 20, 2022

/rtm

@derberg derberg merged commit a0243e7 into asyncapi:master Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. 📑 docs gsod This label should be used for issues or discussions related to ideas for Google Season of Docs ready-to-merge
Projects
Status: Done 🚀
Development

Successfully merging this pull request may close these issues.

4 participants